Skip to content
This repository was archived by the owner on Feb 2, 2023. It is now read-only.

Add ASViewController#660

Merged
appleguy merged 6 commits intofacebookarchive:masterfrom
nguyenhuy:ASViewController
Sep 17, 2015
Merged

Add ASViewController#660
appleguy merged 6 commits intofacebookarchive:masterfrom
nguyenhuy:ASViewController

Conversation

@nguyenhuy
Copy link
Copy Markdown
Contributor

I also update Multiplex sample to use the new ASViewController.

@nguyenhuy
Copy link
Copy Markdown
Contributor Author

This is more like a proof of concept. It's ok to not merge it :P

Comment thread AsyncDisplayKit/ASViewController.m Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I think this effectively does synchronous measurement here and means that just barely in time for -layout, there will be cached sizes.

What would happen if measureWithSizeRange were called on a background thread (on the node) and were still running at this point? Interestingly there haven't been other cases in usage (either framework or app) where I have needed to consider that.

Lastly, small nit, but always create a local variable for a structure that needs to be accessed twice. CGSize boundsSize = self.view.bounds.size;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is synchronous measurement. If it is instead asynchronous and still running at this point, the (backing) view will have the correct size (set previously) but look empty, because the node doesn't layout its subnodes. Once layout is done, we can call setNeedsLayout and subviews will appear correctly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — it's totally fine for this to be synchronous. Later, we can add the API to allow a user to trigger preparation of the view in advance, which would include layout. My question is - what if a user dispatch_async'd a call to measure:, and then pushed the view controller?

The answer may be that we get bad behavior. That's also not a big deal right now, as it would be unusual to manually call measure: in that way and then push the view — but it will be a key part of the final version of 2.0 to support kicking off asynchronous layout, and then having a call that is able to block on the remainder of it (e.g. "-ensureMeasurementComplete" or something — that is probably not a good name). It would return immediately if done, otherwise wait on the background operation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for misunderstood your question. That's indeed a tricky question. Yeah I think the behaviour would be undefined, at least for the current implementation. My main reason is that the async layout pass can finished right before or after any of these events: VC is pushed, loadView, viewWillLayoutSubviews, etc. If we block on the remainder in, say, viewDidAppear, we may be able to do final set up there in case the background operation missed these events.

(There is a high chance that I'm talking non-sense here, please take it with a grain of salt haha)

@nguyenhuy
Copy link
Copy Markdown
Contributor Author

Nits picked.

@nguyenhuy nguyenhuy force-pushed the ASViewController branch 2 times, most recently from a289b5e to 571f257 Compare September 16, 2015 20:50
appleguy added a commit that referenced this pull request Sep 17, 2015
@appleguy appleguy merged commit f8e43ea into facebookarchive:master Sep 17, 2015
peter-iakovlev pushed a commit to peter-iakovlev/AsyncDisplayKit that referenced this pull request Jan 9, 2018
…ookarchive#660)

* Make section mapping work even for empty sections

* Unlock more cases and update changelog

* Fix complexity documentation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants